Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement Maybe.switchIfEmpty(Single) #5582

Merged
merged 3 commits into from
Sep 4, 2017
Merged

Conversation

bmaslakov
Copy link
Contributor

Adds Maybe.switchIfEmpty(Single), fixes #4544.

@codecov
Copy link

codecov bot commented Sep 2, 2017

Codecov Report

Merging #5582 into 2.x will decrease coverage by 0.02%.
The diff coverage is 97.36%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5582      +/-   ##
============================================
- Coverage     96.15%   96.13%   -0.03%     
- Complexity     5821     5825       +4     
============================================
  Files           631      632       +1     
  Lines         41421    41459      +38     
  Branches       5739     5742       +3     
============================================
+ Hits          39830    39858      +28     
- Misses          630      645      +15     
+ Partials        961      956       -5
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/reactivex/Maybe.java 100% <100%> (ø) 169 <1> (+1) ⬆️
...rnal/operators/maybe/MaybeSwitchIfEmptySingle.java 97.22% <97.22%> (ø) 2 <2> (?)
...nternal/operators/observable/ObservableCreate.java 79.48% <0%> (-18.81%) 2% <0%> (ø)
...in/java/io/reactivex/subjects/BehaviorSubject.java 86.97% <0%> (-5.73%) 57% <0%> (ø)
.../io/reactivex/internal/functions/ObjectHelper.java 94.73% <0%> (-5.27%) 20% <0%> (-1%)
...rnal/operators/flowable/FlowableFlatMapSingle.java 93.47% <0%> (-3.27%) 2% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 58.86% <0%> (-2.84%) 31% <0%> (-2%)
...tivex/internal/schedulers/TrampolineScheduler.java 92.3% <0%> (-2.57%) 6% <0%> (ø)
...a/io/reactivex/internal/queue/MpscLinkedQueue.java 98.03% <0%> (-1.97%) 17% <0%> (-1%)
...rnal/subscriptions/DeferredScalarSubscription.java 98.46% <0%> (-1.54%) 28% <0%> (-1%)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14bebc5...4dadbb9. Read the comment docs.

@@ -37,16 +37,6 @@ public void empty() {
}

@Test
public void defaultIfEmptyNonEmpty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why were these removed?

*/
@CheckReturnValue
@SchedulerSupport(SchedulerSupport.NONE)
public final Maybe<T> switchIfEmpty(SingleSource<? extends T> other) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue mentioned Single as return type.

* the alternate SingleSource to subscribe to if the main does not emit any items
* @return a Maybe that emits the items emitted by the source Maybe or the item of an
* alternate SingleSource if the source Maybe is empty.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the appropriate experimental tags and annotations:

@since 2.1.4 - experimental

* alternate SingleSource if the source Maybe is empty.
*/
@CheckReturnValue
@SchedulerSupport(SchedulerSupport.NONE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @Experimental

@@ -24,11 +24,11 @@
*
* @param <T> the value type
*/
public final class MaybeSwitchIfEmpty<T> extends AbstractMaybeWithUpstream<T, T> {
public final class MaybeSwitchIfEmptyToMaybe<T> extends AbstractMaybeWithUpstream<T, T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep the original name, less change per PR, and name the new version MaybeSwitchIfEmptySingle.

@@ -24,7 +24,7 @@
import io.reactivex.processors.PublishProcessor;
import io.reactivex.schedulers.Schedulers;

public class MaybeSwitchIfEmptyTest {
public class MaybeSwitchIfEmptyToMaybeTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, please keep the original name to this test.

@@ -2213,7 +2213,7 @@ public final T blockingGet(T defaultValue) {
@SchedulerSupport(SchedulerSupport.NONE)
public final Maybe<T> defaultIfEmpty(T defaultItem) {
ObjectHelper.requireNonNull(defaultItem, "item is null");
return switchIfEmpty(just(defaultItem));
return switchIfEmpty(Single.just(defaultItem));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this changed, the original just should still work?!

Copy link
Contributor Author

@bmaslakov bmaslakov Sep 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course the original just still works. Just thought that it would be better to wrap the T defaultItem into Single instead of Maybe because the defaultItem cannot be empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore the original state. The less change to existing and practically unrelated code regarding the PR the better.

@akarnokd
Copy link
Member

akarnokd commented Sep 2, 2017

I'm only 50% convinced for the need of this overload.

@akarnokd akarnokd dismissed their stale review September 2, 2017 19:57

reevaluating

@akarnokd
Copy link
Member

akarnokd commented Sep 2, 2017

/cc @vanniktech @artem-zinnatullin

Copy link
Contributor

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, few comments


final SingleObserver<? super T> actual;

final AtomicReference<Disposable> parent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OtherSingleObserver can extend AtomicReference to save field, should we do that @akarnokd?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at L95, you'll see that parent is SwitchIfEmptyMaybeObserver which is declared as AtomicReference<Disposable> thus this is not a new instace of an AtomicReference here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw that, but wouldn't it save field anyway? Though it's super nit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downstream can interact with only one replaceable reference to the original or the alternative disposable, so you'll need an extra field either way.


@Test
public void nonEmpty() {
Maybe.just(1).switchIfEmpty(Single.just(2)).test().assertResult(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertValuesOnly() @vanniktech?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSuccess translates to onNext + onComplete in TestObserver so this requires assertResult.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssertValuesOnly is mostly for Observable & Flowable

@Test
public void error() {
Maybe.<Integer>error(new TestException()).switchIfEmpty(Single.just(2))
.test().assertFailure(TestException.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to also check that no values were emitted to check that it didn't switch to fallback Single

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertFailure does check values:

public final U assertFailure(Class<? extends Throwable> error, T... values) {
    return assertSubscribed()
            .assertValues(values)
            .assertError(error)
            .assertNotComplete();
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes assertFailure does the job under the hood

Copy link
Collaborator

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maybe.switchIfEmpty(Single) method
4 participants